Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

担当が決まっていない提出物にメンターがコメントをした場合にアラートを出す #7953

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

Judeeeee
Copy link
Contributor

@Judeeeee Judeeeee commented Jul 9, 2024

Issue

概要

現在、担当が決まっていない提出物にメンターがコメントをした際には、緑のFlashで"担当になりました"と通知が出ます。
通知を見逃しがちなので、今回の変更ではブラウザのアラート表示後に緑のFlashで通知を出しました。

変更確認方法

  1. feature/alert-dialog-on-unassigned-productsをローカルに取り込む
  2. foreman start -f Procfile.devでローカルサーバーを立ち上げる
  3. 管理者でログインする(ID: komagata)
  4. 提出物一覧ページにアクセスする
  5. 担当が決まっていない提出物の詳細ページにアクセスする
  6. ページ下部のコメントフォームからコメントを投稿する

確認事項

  • コメントをする提出物の担当が決まっていない状態でコメントをすると以下が発生すること
    • "提出物の担当になりました。"の文言をもつブラウザのアラートが表示されること
    • アラートのOKボタンを押下するとアラートの表示が消えること
    • アラートの表示が消えると"担当になりました"の文言を持つ緑のFlashが表示されること

Screenshot

修正前

担当が決まっていない提出物にコメントをすると緑のFlashが出る

2024-07-10.19.00.42.mov

今回の修正

アラートのOKボタン押下後に緑のFlashが出る

2024-07-11.11.07.48.mov

@Judeeeee Judeeeee changed the title 担当が決まっていない提出物にメンターがコメントをした場合にアラートを出す [WIP] 担当が決まっていない提出物にメンターがコメントをした場合にアラートを出す Jul 9, 2024
@Judeeeee Judeeeee self-assigned this Jul 9, 2024
@Judeeeee Judeeeee changed the title [WIP] 担当が決まっていない提出物にメンターがコメントをした場合にアラートを出す 担当が決まっていない提出物にメンターがコメントをした場合にアラートを出す Jul 11, 2024
@Judeeeee Judeeeee marked this pull request as ready for review July 11, 2024 02:48
@Judeeeee
Copy link
Contributor Author

@reckyy

お疲れ様です!
お忙しいところ恐縮ですが、こちらのPRのレビューをお願いできますでしょうか👀
ご都合がつかなければ遠慮なく仰っていただけると幸いですー🙏

@reckyy
Copy link
Contributor

reckyy commented Jul 11, 2024

@Judeeeee
お疲れ様です!
承知しました〜、一週間以内にはレビューさせて頂きます。(もしお急ぎなら、他の方に依頼をお願いいたします。)
よろしくお願いいたします🙇

@Judeeeee
Copy link
Contributor Author

@reckyy

早速のコメント助かります🙏
承知いたしました!ありがとうございます🙇
急ぎではないので、reckyyさんのご都合の良いタイミングで確認いただけると幸いです〜

引き続きよろしくお願いいたしますー!

@Judeeeee Judeeeee requested a review from reckyy July 11, 2024 06:13
Copy link
Contributor

@reckyy reckyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Judeeeee
お待たせしております。
コード、動作確認共に確認させていただきました。
自分からはOKです🙆

------追記------
Approveした後で申し訳ないですが、1点だけコメントしています。
ただ、どちらでもいい話なのでスルーでも大丈夫です🙆
Approveには変わりないので、komagataさんにレビュー依頼していただいて差し支えありません。🙏

@@ -81,6 +81,9 @@ export default {
this.id = json.checker_id
this.name = json.checker_name
if (this.id !== null) {
if (commented) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nits] Approveした後で恐縮ですが、以下の点で少しだけ個人的に違和感があります。

  • commentedという変数名

このcheckProduct関数が担当するボタンでも使われていることから、 commented(コメントがされているか、されてなければ担当がいないだろう、という意味だと解釈しました。)とされたと思います。
ただ、コメントから担当するという意味で、isChargeFromCommentとかでもいいのかなと思いました。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reckyy

レビューありがとうございます🙏
確かに仰る通りですね!ご指摘助かります!
isChargeFromCommentの方がよりよいと思うので、修正いたします🫡

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

お疲れ様です!
ee4d380のcommitで修正いたしましたので、ご確認ください🙏

commentedからisChargeFromCommentへ変更
ラベル・文頭のマーク切り替えはproductCheckerIdの条件分岐で行っている。
そのため、背景もこの仕様に合わせた。
@Judeeeee
Copy link
Contributor Author

@reckyy

先日はレビューありがとうございました!
レビュー修正中に発見した問題があったので共有させてください🙏

提出物の「担当するボタン」の表示についてです。
ボタン単体を押下した際には、「担当から外れる」の表記と黄色の背景に変化します。

しかし、今回の修正(担当者がいない提出物にメンターがコメントをした場合)ではボタン単体の挙動に合わせられていませんでした。

  • 変更前: 「担当から外れる」の文言は変わるが、背景が白色のまま
  • 変更後: 文言、背景共に変化(ボタン単体を押下した場合と同一)

動画

ボタン単体で押下した場合の表示

2024-07-16.15.02.54.mov

変更前

default.mov

変更後

default.mov

お手数おかけして大変恐縮なのですが、修正commitを取り込みご確認いただけないでしょうか?


すみません、よろしくお願いいたします🙇

@Judeeeee Judeeeee requested a review from reckyy July 16, 2024 06:05
@reckyy
Copy link
Contributor

reckyy commented Jul 16, 2024

@Judeeeee
修正確認できました! 👍

また、デザインの問題についてもご共有ありがとうございます。
こちら本来なら自分が気づかないといけないところを、、すみません 🙇
最新を取り込んで確認できました!
僕からはOKです〜🙆

@Judeeeee
Copy link
Contributor Author

@reckyy
とんでもないですー💦
ご確認ありがとうございました〜!

@Judeeeee
Copy link
Contributor Author

@komagata

お疲れ様ですー!
チームメンバーからapproveいただいたので、お手隙の際にレビューしていただけると幸いです🙏

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確認させて頂きました。OKです〜🙆‍♂️

@komagata komagata merged commit aba8ff5 into main Jul 19, 2024
4 checks passed
@komagata komagata deleted the feature/alert-dialog-on-unassigned-products branch July 19, 2024 07:43
@github-actions github-actions bot mentioned this pull request Jul 19, 2024
13 tasks
@machida
Copy link
Member

machida commented Aug 8, 2024

本番で確認しました🎉🎉🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants